Skip to content

fix(plan): decide amend-vs-narrow by pattern containment, not tree snapshot - #12

Merged
jordonpeterson merged 1 commit into
mainfrom
fix/pattern-containment-not-tree-snapshot
Aug 3, 2026
Merged

fix(plan): decide amend-vs-narrow by pattern containment, not tree snapshot#12
jordonpeterson merged 1 commit into
mainfrom
fix/pattern-containment-not-tree-snapshot

Conversation

@jordonpeterson

@jordonpeterson jordonpeterson commented Aug 3, 2026

Copy link
Copy Markdown
Owner

The bug

Assigning an owner to all build.gradle files gave that owner an entire directory.

# before
* @team_a
path/app/* @team_b

# add_owner(*.gradle, @gradle) produced
* @team_a
*.gradle @team_a @gradle
path/app/* @team_b @gradle      # <-- @gradle now owns the whole path

The planner amended a rule in place whenever every tracked file that rule won was inside the scope. That's a property of the current commit — but a CODEOWNERS rule governs files that don't exist yet. path/app/build.gradle happened to be the only tracked file under path/app/, so the rule looked scope-sized and got widened.

This is the amend decision, which #8 did not touch — #8 fixed the derivation side (intersectPattern). Verified against current main: the case above still reproduces, as do the two below.

The same snapshot reasoning was in remove_owner, and in set_owners via matchSetEquals, where it silently transferred a whole directory's ownership away from its owner:

# main today: set_owners(/svc/sub/, [@g]) over "* @a / /svc/ @b"
* @a
/svc/ @g          # <-- @b lost the entire /svc/ tree

The fix

pattern.Contains(outer, inner) — sound containment of the pattern languages. Patterns are tokenized into segment sequences using the same normalization the matcher compiles (extracted as normalizeSegs, so the two can't drift), then walked for containment. It's deliberately incomplete: false means "unproven", never "disjoint".

Two subtleties the brute-force soundness test caught, both of which had produced wrong answers:

  • A trailing slash does not anchor a single-segment pattern. docs/ gets an implicit leading **/, so it matches web/docs/spec.md. A textual prefix check read it as root-anchored and amended it for the scope /docs/.
  • A final * compiles through a different arm than a final glob. /x/* matches exactly one level, while /x/foo also matches everything beneath it.

This builds on #8 rather than replacing it: deriveIntersection is #8's intersectPattern unchanged, including its anchoring-aware ruleDirPrefix and scopeContainedInRule. The new layer proves each candidate stays inside both the scope and the rule before returning it.

# after
* @team_a
*.gradle @team_a @gradle
path/app/* @team_b
path/app/*.gradle @team_b @gradle

One disclosed compromise

CODEOWNERS cannot express dir/**.gradle: there's no way to say "one level and matching the glob", because dir/*.gradle also matches files under a directory named .gradle. Ordering can't resolve it either — both paths match both patterns, so last-match-wins moves them together.

#8's ruleDirPrefix rejects dir/* rules outright, which would make the reported case refuse once the amend bug is fixed. So the narrowing rule is emitted with a warning naming the residual. It can never leak the new owner out of scope or withhold it where due; only a co-owner can be wrong. Where the rule's reach is expressible (/svc/svc/**/*.gradle) the derivation is proven and silent.

Verification

  • make all green — 144 tests, including all of plan: derive narrowing for file-glob scopes across directory rules (shape 4) #8's test files unmodified
  • Existing tests unchanged, all additions (plan_test.go: +275 / −0)
  • 8 of the 9 new planner tests fail on unmodified main; the 9th (TestR6_DirectoryNameScopeConverges) passes and is kept as a regression guard
  • 500k-case differential fuzz vs. the hmarr/codeowners oracle: 0 mismatches
  • plan → apply → plan is idempotent (exit 1) across every new derivation shape
  • internal/pattern/contains_test.go brute-forces soundness over a pattern × path universe

Follow-ups (not in this PR)

Two pre-existing snapshot bugs of the same family, left alone because they change the tool's contract (the invariants are defined over the tracked tree):

  • synthSet's recapture check inserts above a rule that will capture future in-scope files
  • granting a directory only covers depths that already contain files, so deeper files added later never get the owner

🤖 Generated with Claude Code

Copilot AI review requested due to automatic review settings August 3, 2026 19:19

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a planner correctness bug where amend-vs-insert decisions were based on the current tracked tree (a snapshot), which could incorrectly widen CODEOWNERS rules and change ownership for future (currently untracked) files. It replaces snapshot-based reasoning with sound (but intentionally incomplete) semantic pattern-language containment, and tightens narrowing derivations by proving them before emission (or disclosing residual risk when exact intersection is not expressible).

Changes:

  • Replace tree-snapshot “subset” checks with semantic pattern.Contains(outer, inner) for amend-in-place decisions in add/remove flows, and for exact-scope amends in set_owners.
  • Rework narrowing derivation to generate candidate intersection patterns and prove them against both the tracked tree and (when possible) pattern-language containment; emit warnings for inexact-but-tree-exact narrowings.
  • Add extensive test coverage for the reported bug shapes and the new containment semantics, and document the new tests/spec assertions.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
internal/plan/plan.go Switches amend/narrow decisions to semantic containment; introduces proven (or disclosed) narrowing derivations and removes snapshot-based helpers.
internal/plan/plan_test.go Adds regression tests covering snapshot-widening bugs, containment edge cases, inexact narrowing disclosure, and convergence fixes.
internal/pattern/pattern.go Extracts normalizeSegs so containment tokenization shares the matcher’s normalization rules.
internal/pattern/contains.go Introduces sound pattern-language containment (Contains) used by the planner for safe amend decisions.
internal/pattern/contains_test.go Brute-force soundness checks and a corpus of known containment/non-containment pairs to lock in semantics.
docs/BEHAVIOR.md Documents the newly added behavioral tests/spec assertions and updates the documented test count.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread internal/pattern/contains.go
…apshot

The planner chose to amend a rule in place whenever every TRACKED file that
rule won was inside the operation's scope. That is a property of the current
commit, but a CODEOWNERS rule governs files that do not exist yet.

Reported case: with `path/app/build.gradle` the only tracked file under
`path/app/`, `add_owner(*.gradle, @team)` amended `path/app/* @b` in place,
handing @team every future non-gradle file in that directory. The same
snapshot reasoning was in remove_owner, and in set_owners via matchSetEquals,
where it silently transferred an entire directory's ownership away.

Replaces the check with pattern.Contains — sound containment of the pattern
LANGUAGES. Patterns are tokenized into segment sequences using the same
normalization the matcher compiles (extracted as normalizeSegs so the two
cannot drift), then walked for containment. Contains is deliberately
incomplete: false means "unproven", never "disjoint".

Two subtleties the brute-force soundness test caught:

  - A trailing slash does NOT anchor a single-segment pattern. `docs/` gets
    an implicit leading `**/`, so it matches `web/docs/spec.md` — a textual
    prefix comparison read it as root-anchored and amended it for the scope
    `/docs/`.
  - A final `*` compiles through a different arm than a final glob, so
    `/x/*` matches exactly one level while `/x/foo` also matches everything
    beneath it.

Derived narrowing patterns are now proven rather than trusted: a candidate
must stay inside both the scope and the rule, and match exactly the right
tracked paths. That turns the junk derivations for directory-name scopes
(`x`, `docs`), which removePass re-inserted every pass before reporting
"did not converge", into ordinary amends.

CODEOWNERS cannot express `dir/*` ∩ `*.gradle` — there is no way to say "one
level AND matching the glob", since `dir/*.gradle` also matches under a
DIRECTORY named `.gradle`. Refusing would make add_owner(*.gradle, ...)
impossible on any repo with a `dir/*` rule, so the narrowing rule is emitted
with a warning disclosing the residual. It can never leak the new owner out
of scope or withhold it; only a co-owner can be wrong. Where the rule's reach
is expressible (`/svc` -> `svc/**/*.gradle`) the derivation is proven and
silent.

Existing tests are unchanged; all additions. Verified with `make all`, the
500k-case differential fuzz against the hmarr oracle (0 mismatches), and
plan/apply/plan idempotence across the new derivation shapes.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@jordonpeterson
jordonpeterson force-pushed the fix/pattern-containment-not-tree-snapshot branch from 28ba6df to a0a9737 Compare August 3, 2026 19:39
@jordonpeterson
jordonpeterson merged commit 49cdfd8 into main Aug 3, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants